Repository navigation
Conversation
… sheet
Two related changes to the full emoji library sheet — both shippable
independently of the catalog generator and the "More reactions" hit
target work.
Skin-tone selector: a single horizontal row of 6 swatches (default + 5
Fitzpatrick tones) sits between the search field and the tab strip.
Tapping a swatch persists the choice in `UserDefaults` via
`EmojiSkinTonePreference` and every tone-capable cell in the grid
re-renders in that tone immediately. The mechanism matches what other
clients do — a single global preference rather than per-pick popovers,
which conflicted with `Button`'s tap recognizer when tried first.
`SkinTone.swift` introduces:
- `EmojiSkinTone` enum mapping each tone to its Fitzpatrick modifier
codepoint, with a preview-swatch helper.
- `EmojiSkinTonePreference` wrapper around `UserDefaults` that posts a
notification when the value changes so live sheets refresh.
- `String.applyingSkinTone(_:)` and `removingSkinTones()` helpers that
handle both plain base emoji (append modifier) and single-human-glyph
ZWJ sequences (insert modifier before the first ZWJ, with VS-16
awareness). Multi-person sequences are intentionally out of scope.
- `EmojiToneCapability` — curated `Set<String>` of tone-capable base
emoji. Membership check strips VS-16 so "👋" and "👋\u{FE0F}" both
match.
Grid scroll-cancellation: the emoji + custom-image cell wrappers now
use `.contentShape(Rectangle()).onTapGesture` instead of `Button { }
.buttonStyle(.plain)`. `Button` inside a `LazyVGrid` inside a
`ScrollView` only cancels its tap on substantial finger movement, so a
quick flick to scroll the long grid was registering as a pick on
release. `.onTapGesture` cancels on any drag past the hit-test
threshold, which is what the user expects for a scroll.
Closes #304.
Owner
|
Thank you for the suggestion! We do not accept feature requests via issues — please submit a pull request instead. Closing. |
pbxproj conflicts were registration-line collisions only (SkinTone.swift vs WispTopHeader/AccountSwitcherSheet added in the same spots); kept both.
barrydeen
requested changes
Oct 1, 2026
barrydeen
left a comment
barrydeen
left a comment
Owner
There was a problem hiding this comment.
PR Review: #345 — feat(emoji): skin-tone picker + tap-fix in emoji library
Verdict: CHANGES REQUESTED
Risk areas touched: user-visible picker UX, accessibility, search/compose input path (toned emoji now enter note content), build (new root file + pbxproj)
Scope check: Files match the description — SkinTone.swift (new, 174 lines), EmojiLibrarySheet.swift (grid rework + tone row), project.pbxproj (+4).
Build/tests: no Swift toolchain on this machine and no CI checks on the branch, so this review is static; recommend one simulator pass of the library sheet before merge.
Findings
[MEDIUM] Emoji grid cells lost VoiceOver activation when Button was replaced by Text + onTapGesture
- Where:
EmojiLibrarySheet.swift(grid cell:Text(unicode).contentShape(Rectangle()).onTapGesture { handleUnicodePick(rendered) }) - What: The old cells were
Buttons, which VoiceOver activates natively (double-tap). PlainTextwith.onTapGestureexposes no activation action, so the grid's core function is unreachable with VoiceOver on — a regression introduced while fixing the scroll misfire (#304). - Impact / trigger: VoiceOver users can no longer insert emoji from the library sheet at all.
- Fix: keep the tap gesture and add back the semantics:
.accessibilityLabel(name).accessibilityAddTraits(.isButton).accessibilityAction { handleUnicodePick(rendered) }. The misfire bug was the Button reacting to a tap that the scroll should have cancelled; a gesture-driven action with explicit a11y traits keeps the fix intact.
Questions for the author
SkinTone.swiftEmojiToneCapability— 🤳 (selfie) is tone-capable per Unicode but I don't see it in the base set; and the ZWJ insertion path only meaningfully fires for single-codepoint human bases (multi-person/role sequences like 👨👩👧 or 👨🚀 aren't in the capability set, so the branch is close to dead code today). Intentional curation, or oversight? If intentional, worth a one-line comment so the next person doesn't "fix" it.- The preferred tone is one global UserDefaults key (
com.wisp.emoji.preferredSkinTone), not per-account. Fine for a UI pref — just confirming that's deliberate given account switching elsewhere is per-pubkey.
Nits
SkinTone.swiftEmojiSkinToneRow— the five swatch buttons have no.accessibilityLabel(VoiceOver announces them as bare "👋, button" ×5 with no way to tell tones apart), and the preview glyph is 👋 for every tone. Per-tone labels ("Medium-light skin tone") would fix both complaints cheaply.SkinTone.swift—String.applyingSkinTonere-appends trailing VS-16 after inserting the modifier; correct per UTS #51, but 👋+🏽 vs ⛹+🏾 exercise both branches — worth 2 Swift Testing cases inwispTestssince this is pure logic and CI-runnable.
Checked and OK
SkinTone.swiftregistered correctly inproject.pbxproj: exactly onePBXFileReference, onePBXBuildFile, added to the app target's Sources phase only — matches how the other root-level view files (EmojiLibrarySheet.swift,EmojiData.swift) are wired; the test/UITests/ShareExtension targets don't see it, consistent with their existing contents.- Tone-modifier codepoints are U+1F3FB–U+1F3FF, mapping order default→light→…→dark is correct, and the default swatch sends the unmodified base.
- Capability check strips VS-16 before lookup, so VS-16-required bases (⛹, 🏃) resolve correctly, and
applyingSkinTonere-adds VS-16 for those — the qualified/neutral-form distinction is handled per UTS #51. - ZWJ insertion goes after the first human sequence element, not appended blindly — safe for the single-human cases that are actually in the capability set.
- Preference writes go through one UserDefaults key and post
.emojiSkinTonePreferenceDidChange; the sheet'sonReceiverefreshes@Stateso a tone change in Settings reflects in an open sheet — no stale-state path I could find. - Search path also routes through the tone-aware cell, so query results insert toned emoji consistently with browse results;
handleUnicodePick(rendered)still feeds the same insertion path as before (toned emoji were already typeable via the system keyboard / possible via Amethyst reactions, so no new relay-compat surface). - The tap fix itself is sound:
onTapGestureis cancelled by the enclosingScrollView's drag, which is precisely the #304 misfire behavior theButtonstyle couldn't get. - No keys/crypto/relay/storage-schema surfaces touched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related changes to the full emoji library sheet — both shippable independently of the catalog generator and the "More reactions" hit-target work.
Skin-tone selector: a single horizontal row of 6 swatches (default + 5 Fitzpatrick tones) sits between the search field and the tab strip. Tapping a swatch persists the choice in
UserDefaultsviaEmojiSkinTonePreferenceand every tone-capable cell in the grid re-renders in that tone immediately. The mechanism matches what other clients do — a single global preference rather than per-pick popovers, which conflicted withButton's tap recognizer when tried first.Grid scroll-cancellation: the emoji and custom-image cell wrappers now use
.contentShape(Rectangle()).onTapGestureinstead ofButton { } .buttonStyle(.plain).Buttoninside aLazyVGridinside aScrollViewonly cancels its tap on substantial finger movement, so a quick flick to scroll the long grid was registering as a pick on release..onTapGesturecancels on any drag past the hit-test threshold, which is what the user expects for a scroll.SkinTone.swiftintroduces:EmojiSkinToneenum mapping each tone to its Fitzpatrick modifier codepoint, with a preview-swatch helper.EmojiSkinTonePreferencewrapper aroundUserDefaultsthat posts a notification when the value changes so live sheets refresh.String.applyingSkinTone(_:)andremovingSkinTones()helpers that handle both plain base emoji (append modifier) and single-human-glyph ZWJ sequences (insert modifier before the first ZWJ, with VS-16 awareness). Multi-person sequences are intentionally out of scope.EmojiToneCapability— curatedSet<String>of tone-capable base emoji. Membership check strips VS-16 so "👋" and "👋\u{FE0F}" both match.Files
SkinTone.swift— new, ~135 linesEmojiLibrarySheet.swift—skinToneSelectorrow,toneAwareCellrendering,preferredTonestate + notification observer;Button→.onTapGestureswap on both gridswisp.xcodeproj/project.pbxproj— registersSkinTone.swiftTest plan
Closes #304.